Repository navigation
Fix workspace close shortcut window targeting - #6365
azooz2003-bit wants to merge 6 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesFocused-window close-workspace routing and telemetry
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 2 warnings)
✅ Passed checks (17 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes Cmd+Shift+W so it closes the workspace in the focused/key window rather than whatever window the active-manager happened to hold last, and anchors the confirmation sheet to that same window. It adds a new
Confidence Score: 4/5The shortcut-routing fix itself is correct and well-scoped, but the new The window-targeting logic, visibility widenings, and test additions are all sound. The one issue is Sources/TabManager+CloseConfirmationTelemetry.swift — should move to a debug folder or test-support module rather than Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Cmd+Shift+W pressed"] --> B["matchConfiguredShortcut(.closeWorkspace)"]
B --> C["closeWorkspaceFromFocusedShortcut(event:)"]
C --> D["mainWindowContextForFocusedWorkspaceCloseShortcut(event:)"]
D --> E{NSApp.keyWindow is cmux window?}
E -- yes --> F["Use keyWindow context"]
E -- no --> G{NSApp.mainWindow is cmux window?}
G -- yes --> H["Use mainWindow context"]
G -- no --> I["mainWindowContext(forShortcutEvent:)"]
I --> J{Found context?}
J -- yes --> K["Use event-derived context"]
J -- no --> L{shortcutEventHasAddressableWindow?}
L -- yes & cmuxWindowShouldOwnClose --> M["preferredMainWindowContextForShortcutRouting"]
L -- no / not cmux --> N["return nil → no-op"]
F --> O["setActiveMainWindow / activateMainWindowContext"]
H --> O
K --> O
M --> O
O --> P["tabManager.closeCurrentWorkspaceWithConfirmation()"]
P --> Q["recordCloseConfirmationTarget (#if DEBUG only)"]
P --> R["presentAlert on closeConfirmationPresentingWindow"]
R --> S{presentation}
S -- sheet --> T["recordCloseConfirmationSheetPresentation (#if DEBUG only)"]
S -- appModal --> U["recordCloseConfirmationAppModalPresentation (#if DEBUG only)"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["Cmd+Shift+W pressed"] --> B["matchConfiguredShortcut(.closeWorkspace)"]
B --> C["closeWorkspaceFromFocusedShortcut(event:)"]
C --> D["mainWindowContextForFocusedWorkspaceCloseShortcut(event:)"]
D --> E{NSApp.keyWindow is cmux window?}
E -- yes --> F["Use keyWindow context"]
E -- no --> G{NSApp.mainWindow is cmux window?}
G -- yes --> H["Use mainWindow context"]
G -- no --> I["mainWindowContext(forShortcutEvent:)"]
I --> J{Found context?}
J -- yes --> K["Use event-derived context"]
J -- no --> L{shortcutEventHasAddressableWindow?}
L -- yes & cmuxWindowShouldOwnClose --> M["preferredMainWindowContextForShortcutRouting"]
L -- no / not cmux --> N["return nil → no-op"]
F --> O["setActiveMainWindow / activateMainWindowContext"]
H --> O
K --> O
M --> O
O --> P["tabManager.closeCurrentWorkspaceWithConfirmation()"]
P --> Q["recordCloseConfirmationTarget (#if DEBUG only)"]
P --> R["presentAlert on closeConfirmationPresentingWindow"]
R --> S{presentation}
S -- sheet --> T["recordCloseConfirmationSheetPresentation (#if DEBUG only)"]
S -- appModal --> U["recordCloseConfirmationAppModalPresentation (#if DEBUG only)"]
Reviews (9): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func workspaceIds(inWindow windowId: String) -> [String]? { | ||
| guard socketCommand("focus_window \(windowId)") == "OK", | ||
| let response = socketCommand("list_workspaces") else { | ||
| return nil | ||
| } | ||
| if response == "No workspaces" { | ||
| return [] | ||
| } | ||
| return response | ||
| .split(separator: "\n") | ||
| .compactMap { line in | ||
| line | ||
| .split(whereSeparator: { $0 == " " || $0 == "\t" }) | ||
| .map(String.init) | ||
| .first(where: { UUID(uuidString: $0) != nil }) | ||
| } | ||
| } |
There was a problem hiding this comment.
workspaceIds sends focus_window as a side effect during polling
workspaceIds(inWindow:) issues a focus_window socket command before list_workspaces every time it is called. Because waitForWorkspace polls this predicate repeatedly (up to its timeout), the app's focused-window state is mutated on every predicate iteration. The two sequential waitForWorkspace calls at the end of the test — one for focusedWindowId and one for otherWindowId — will alternate sending focus_window to each window until both settle. While harmless for this specific test (both windows remain open throughout polling), any future test that calls waitForWorkspace while relying on a stable focused window, or while an action is in progress on the currently-focused window, could trigger unexpected behavior or race conditions in the app under test.
| } | ||
|
|
||
| private func keyWindowId() -> String? { | ||
| guard let response = socketCommand("list_windows") else { return nil } | ||
| for line in response.split(separator: "\n") { | ||
| let parts = line | ||
| .trimmingCharacters(in: .whitespacesAndNewlines) | ||
| .split(separator: " ") | ||
| .map(String.init) | ||
| guard parts.first == "*", parts.count >= 3 else { continue } | ||
| return parts[2] | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| private func waitForWorkspace( | ||
| _ workspaceId: String, |
There was a problem hiding this comment.
keyWindowId() depends on a hardcoded field-position assumption in list_windows output
keyWindowId() parses the response from list_windows by checking that the first token is "*" and reading the window ID from parts[2] (the third space-split field). If the output format changes — for example, if the * marker is removed, or the window ID moves to a different position — this silently returns nil on every iteration, causing waitForKeyWindow to always time out. The test then invokes app.typeKey("w", modifierFlags: [.command, .shift]) on whatever window happens to be focused rather than the intended focusedWindowId, potentially masking the regression the test is intended to catch.
7b1c500 to
a31e19f
Compare
a31e19f to
bdc2c88
Compare
bdc2c88 to
7513f14
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 6288-6305: The comment about auxiliary cmux windows on lines
6291-6292 is currently placed in the wrong branch. It describes behavior for
windows that should NOT own the close shortcut (auxiliary windows), but it is
located in the branch that handles windows that SHOULD own it (when
cmuxWindowShouldOwnCloseShortcut returns true). Move this comment to the else
branch (where the nil return happens) to correctly document the auxiliary window
case, or relocate it before the outer if statement for the
shortcutEventHasAddressableWindow check to provide overall context for the logic
flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e58d2c3e-ff4c-43ff-88f1-55eb42b17ec6
📒 Files selected for processing (2)
Sources/AppDelegate.swiftSources/TabManager.swift
| if shortcutEventHasAddressableWindow(event) { | ||
| if let eventWindow = resolvedShortcutEventWindow(event), | ||
| cmuxWindowShouldOwnCloseShortcut(eventWindow) { | ||
| // Auxiliary cmux windows do not own workspaces. Preserve their existing | ||
| // app-shortcut behavior by delegating to the focused main window below. | ||
| } else { | ||
| #if DEBUG | ||
| logWorkspaceCreationRouting( | ||
| phase: "choose", | ||
| source: "shortcut.closeWorkspace", | ||
| reason: "event_context_required_no_fallback", | ||
| event: event, | ||
| chosenContext: nil | ||
| ) | ||
| #endif | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Comment placement reduces readability of the guard logic.
The comment on lines 6291-6292 describes auxiliary window behavior but is placed in the branch that executes when cmuxWindowShouldOwnCloseShortcut(eventWindow) returns true—i.e., for windows that should own the shortcut (main terminal windows). The actual auxiliary-window case is the else branch that returns nil.
Consider moving the rationale comment before the outer if or adding a brief inline comment in the else branch:
Suggested clarification
+ // Auxiliary cmux windows do not own workspaces. When the shortcut originates
+ // from such a window, we check if it should delegate to the active manager.
if shortcutEventHasAddressableWindow(event) {
if let eventWindow = resolvedShortcutEventWindow(event),
cmuxWindowShouldOwnCloseShortcut(eventWindow) {
- // Auxiliary cmux windows do not own workspaces. Preserve their existing
- // app-shortcut behavior by delegating to the focused main window below.
+ // Main terminal window—fall through to active-manager fallback.
} else {
+ // Auxiliary window or unresolvable—do not perform close action.
`#if` DEBUG📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if shortcutEventHasAddressableWindow(event) { | |
| if let eventWindow = resolvedShortcutEventWindow(event), | |
| cmuxWindowShouldOwnCloseShortcut(eventWindow) { | |
| // Auxiliary cmux windows do not own workspaces. Preserve their existing | |
| // app-shortcut behavior by delegating to the focused main window below. | |
| } else { | |
| #if DEBUG | |
| logWorkspaceCreationRouting( | |
| phase: "choose", | |
| source: "shortcut.closeWorkspace", | |
| reason: "event_context_required_no_fallback", | |
| event: event, | |
| chosenContext: nil | |
| ) | |
| #endif | |
| return nil | |
| } | |
| } | |
| // Auxiliary cmux windows do not own workspaces. When the shortcut originates | |
| // from such a window, we check if it should delegate to the active manager. | |
| if shortcutEventHasAddressableWindow(event) { | |
| if let eventWindow = resolvedShortcutEventWindow(event), | |
| cmuxWindowShouldOwnCloseShortcut(eventWindow) { | |
| // Main terminal window—fall through to active-manager fallback. | |
| } else { | |
| // Auxiliary window or unresolvable—do not perform close action. | |
| `#if` DEBUG | |
| logWorkspaceCreationRouting( | |
| phase: "choose", | |
| source: "shortcut.closeWorkspace", | |
| reason: "event_context_required_no_fallback", | |
| event: event, | |
| chosenContext: nil | |
| ) | |
| `#endif` | |
| return nil | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/AppDelegate.swift` around lines 6288 - 6305, The comment about
auxiliary cmux windows on lines 6291-6292 is currently placed in the wrong
branch. It describes behavior for windows that should NOT own the close shortcut
(auxiliary windows), but it is located in the branch that handles windows that
SHOULD own it (when cmuxWindowShouldOwnCloseShortcut returns true). Move this
comment to the else branch (where the nil return happens) to correctly document
the auxiliary window case, or relocate it before the outer if statement for the
shortcutEventHasAddressableWindow check to provide overall context for the logic
flow.
cd51e00 to
977d1ab
Compare
977d1ab to
83a0167
Compare
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Summary
Testing
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes Cmd+Shift+W so it closes the selected workspace in the focused main window and anchors the confirmation sheet to that same window. Adds a multi‑window XCUITest with key‑window checks and sturdier control‑socket readiness/client.
Written for commit 556c873. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests